feat: multiple otel exporters #415
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughOpenTelemetry exporter configuration migrated from string-based lists and legacy otlp_settings to typed, discriminated exporter objects (console, file, otlp) across schema, config models, tracer, tests, and examples. Tracer now supports sampling (ParentBased + TraceIdRatioBased). Backward-compatibility coercion preserves legacy configs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant Config as Config Loader
participant Settings as OpenTelemetrySettings
participant Tracer as Tracer.setup
participant Provider as TracerProvider
participant Resolver as Exporter Resolver
participant Backend as Backend (Console/File/OTLP)
App->>Config: load mcp_agent.config.yaml
Config-->>App: OpenTelemetrySettings (coerced from legacy if needed)
App->>Tracer: init_tracing(settings)
Tracer->>Provider: create TracerProvider(resource, sampler?)
note right of Provider #f7fbff: Sampler = ParentBased(TraceIdRatioBased(rate)) if configured
loop exporters
Tracer->>Resolver: resolve(entry)
alt type == "console"
Resolver-->>Tracer: ConsoleExporter
Tracer->>Backend: register Console
else type == "file"
Resolver-->>Tracer: FileExporter(path/path_settings)
Tracer->>Backend: register File
else type == "otlp"
Resolver->>Settings: merge endpoint/headers (typed or legacy)
alt endpoint present
Resolver-->>Tracer: OTLPExporter(endpoint, headers)
Tracer->>Backend: register OTLP
else missing
Tracer-->>App: log error (missing endpoint)
end
end
end
App-->>Provider: set global provider
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (19)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/mcp_agent/mcp/mcp_aggregator.py (4)
858-869: Bug: parameter ‘server_name’ is shadowed and discarded.The local assignment resets server_name to None, ignoring a caller-provided server_name.
- server_name: str = None - local_tool_name: str = None - - if server_name: + local_tool_name: str | None = None + if server_name is not None: span.set_attribute("server_name", server_name) local_tool_name = name else: - server_name, local_tool_name = await self._parse_capability_name( + parsed_server_name, local_tool_name = await self._parse_capability_name( name, "tool" ) + server_name = parsed_server_name span.set_attribute("parsed_server_name", server_name) span.set_attribute("parsed_tool_name", local_tool_name)
1406-1419: Bug: returning wrong type from _call_tool.Success path returns
result.content(list), while the method is annotated to return CallToolResult. Also error path returns CallToolResult. Return the full result for consistency.async def _call_tool( self, name: str, arguments: dict | None = None ) -> CallToolResult: """Call a specific tool from the aggregated servers.""" try: result = await self.aggregator.call_tool(name=name, arguments=arguments) - return result.content + return result except Exception as e: return CallToolResult( isError=True, content=[ TextContent(type="text", text=f"Error calling tool: {str(e)}") ], )
1444-1447: Bug: _list_resources returns the wrapper instead of the list.Unwrap
resources.resourceslike the analogous tools/prompts methods.async def _list_resources(self): """List available resources from the connected MCP servers.""" - resources = await self.aggregator.list_resources() - return resources + resources_result = await self.aggregator.list_resources() + return resources_result.resources
536-548: Broken temp connection lifecycle in get_server().Returning a client from inside an
async withcloses it immediately; callers receive a closed session.Minimal safe fix: do not attempt to return a temporary connection; force callers to use context-managed flows.
else: - logger.debug( - f"Creating temporary connection to server: {server_name}", - data={ - "progress_action": ProgressAction.STARTING, - "server_name": server_name, - "agent_name": self.agent_name, - }, - ) - async with gen_client( - server_name, server_registry=self.context.server_registry - ) as client: - return client + logger.debug( + "Non-persistent mode: use gen_client(...) in a context manager to obtain a temporary session.", + data={"server_name": server_name, "agent_name": self.agent_name}, + ) + return NoneIf you want, I can propose an
asynccontextmanagerwrapper to yield a temporary session.examples/tracing/langfuse/README.md (1)
68-76: Fix section numbering (missing step “3”).Change “##
4Run locally” to “##3Run locally” to avoid confusion.-## `4` Run locally +## `3` Run locallysrc/mcp_agent/tracing/tracer.py (1)
208-215: Wrong package name in OpenAI instrumentation hint; severity could be reduced.The message recommends the Anthropic package for OpenAI. Suggest correcting the package and using warning.
- except ModuleNotFoundError: - logger.error( - "OpenAI OTEL instrumentation not available. Please install opentelemetry-instrumentation-anthropic." - ) + except ModuleNotFoundError: + logger.warning( + "OpenAI OTEL instrumentation not available. Please install opentelemetry-instrumentation-openai." + )schema/mcp-agent.config.schema.json (1)
687-693: Typo: “Streamble HTTP” → “Streamable HTTP”.User-facing description string.
Apply this change in the description text.
🧹 Nitpick comments (31)
examples/tracing/temporal/README.md (2)
52-58: Showcase multiple exporters in the example.Since the PR enables multiple exporters, add a second exporter (e.g., console) to make this immediately discoverable.
otel: enabled: true exporters: - type: otlp - endpoint: "http://localhost:4318/v1/traces" + endpoint: "http://localhost:4318/v1/traces" + - type: console
50-50: Use a stable Jaeger docs link to reduce rot.-[Run Jaeger locally](https://www.jaegertracing.io/docs/2.5/getting-started/) and then ensure the `mcp_agent.config.yaml` for this example includes a typed OTLP exporter with the collector endpoint: +[Run Jaeger locally](https://www.jaegertracing.io/docs/latest/getting-started/) and then ensure the `mcp_agent.config.yaml` for this example includes a typed OTLP exporter with the collector endpoint:examples/tracing/agent/README.md (1)
13-23: Docs: show multiple OTLP exporters to highlight the new capability.Consider expanding the snippet to demonstrate two OTLP endpoints (e.g., developer and internal) plus console/file to make the feature obvious.
Apply this diff to the YAML block:
otel: enabled: true exporters: - type: console - - type: file - - type: otlp - endpoint: "http://localhost:4318/v1/traces" + - type: file + - type: otlp + endpoint: "http://localhost:4318/v1/traces" + - type: otlp + endpoint: "https://internal-collector:4318/v1/traces" + headers: + Authorization: "Bearer ${INTERNAL_OTEL_TOKEN}"examples/tracing/langfuse/mcp_agent.secrets.yaml.example (1)
10-10: Clarify header precedence in docs.Since headers are merged, explicitly state which source wins on key conflicts.
Apply this doc tweak:
- # Headers are merged with typed OTLP exporter settings + # Headers are merged with typed OTLP exporter settings (please document precedence, e.g., exporter.headers overrides otel.otlp_settings.headers on key conflicts)examples/tracing/mcp/README.md (1)
51-59: Docs: add in-snippet comments for endpoint scope and optional extra exporters.Small comments make it clear this path is for traces and that multiple exporters are supported.
Apply this diff to the YAML block:
otel: enabled: true exporters: - - type: otlp - endpoint: "http://localhost:4318/v1/traces" + - type: otlp + # Traces endpoint; metrics/logs would use /v1/metrics or /v1/logs respectively + endpoint: "http://localhost:4318/v1/traces" + # Optional: add console/file for local debugging alongside OTLP + # - type: console + # - type: fileexamples/tracing/mcp/mcp_agent.config.yaml (1)
20-22: Consider including a console exporter in the example for easier local debugging.Keeps OTLP for Jaeger while providing immediate on-stdout visibility.
Apply this diff:
otel: enabled: true exporters: + - type: console - type: otlp endpoint: "http://localhost:4318/v1/traces"examples/workflows/workflow_router/mcp_agent.config.yaml (1)
24-28: Showcase multiple exporters (incl. file and multiple OTLP) since that’s the PR goal.Add an example illustrating file plus two OTLP collectors and optional headers/compression.
Apply:
exporters: - - type: console - # To export to a collector, also include: - # - type: otlp - # endpoint: "http://localhost:4318/v1/traces" + - type: console + - type: file + # path_settings are optional; defaults apply if omitted + # path: "logs/traces-{timestamp}.jsonl" + # Multiple collectors are supported: + - type: otlp + endpoint: "http://localhost:4318/v1/traces" + # headers: + # Authorization: "Bearer <token>" + # compression: "gzip" + - type: otlp + endpoint: "https://internal-collector.example.com/v1/traces"examples/tracing/temporal/mcp_agent.config.yaml (1)
48-51: Consider adding explicit file exporter path to avoid ambiguity.Helps users find the trace file without digging into defaults.
Apply:
exporters: - - type: file + - type: file + # path: "logs/otel-traces-{timestamp}.jsonl" - type: otlp endpoint: "http://localhost:4318/v1/traces"examples/tracing/llm/README.md (1)
13-23: Add headers note for secured collectors.Many setups require auth; add a commented example line.
Apply:
- type: otlp endpoint: "http://localhost:4318/v1/traces" + # headers: + # Authorization: "Bearer <token>"examples/workflows/workflow_evaluator_optimizer/mcp_agent.config.yaml (1)
29-33: Include a file exporter example and highlight multi-export.Aligns example with “multiple exporters” capability.
Apply:
exporters: - - type: console - # To export to a collector, also include: - # - type: otlp - # endpoint: "http://localhost:4318/v1/traces" + - type: console + - type: file + # path: "logs/traces-{timestamp}.jsonl" + # To export to a collector, also include one or more: + # - type: otlp + # endpoint: "http://localhost:4318/v1/traces"examples/workflows/workflow_orchestrator_worker/mcp_agent.config.yaml (1)
29-33: Mirror the multi-exporter guidance here too.Add file exporter and clarify multiple OTLP endpoints option.
Apply:
exporters: - - type: console - # To export to a collector, also include: - # - type: otlp - # endpoint: "http://localhost:4318/v1/traces" + - type: console + - type: file + # path: "logs/traces-{timestamp}.jsonl" + # To export to one or more collectors, also include: + # - type: otlp + # endpoint: "http://localhost:4318/v1/traces"examples/tracing/langfuse/mcp_agent.config.yaml (1)
29-33: OTLP endpoint shape and headers: verify schema compatibility.Ensure the new typed exporter expects a full OTLP/HTTP traces URL (with /v1/traces) and not just a host:port; otherwise requests may be double-suffixed or 404. Also confirm that headers from secrets merge correctly for Authorization.
Apply if supported by schema to make intent explicit:
otel: enabled: true exporters: - type: otlp - endpoint: "https://us.cloud.langfuse.com/api/public/otel/v1/traces" + endpoint: "https://us.cloud.langfuse.com/api/public/otel/v1/traces" + # Optional: + # protocol: http/protobuf + # compression: gzip + # timeout_ms: 10000 # Set Authorization header with API key in mcp_agent.secrets.yamlexamples/workflows/workflow_deep_orchestrator/mcp_agent.config.yaml (1)
27-35: File exporter path: ensure directory exists or is auto-created.If the runtime doesn’t auto-create trace directories, consider adding a note or switching to a path under logs/ for consistency with logger.path_settings.
Apply if supported:
- type: file path_settings: - path_pattern: "traces/mcp-agent-trace-{unique_id}.jsonl" + path_pattern: "traces/mcp-agent-trace-{unique_id}.jsonl" # Ensure 'traces/' exists or will be created unique_id: "timestamp" timestamp_format: "%Y%m%d_%H%M%S" # To export to a collector as well, include: # - type: otlp # endpoint: "http://localhost:4318/v1/traces"examples/tracing/llm/mcp_agent.config.yaml (1)
29-34: Nice—typed exporters. Consider adding a file path example for discoverability.The file exporter relies on defaults; a short hint helps users find traces.
exporters: - type: console - - type: file + - type: file + # path_settings: + # path_pattern: "traces/mcp-agent-trace-{unique_id}.jsonl" + # unique_id: "timestamp" + # timestamp_format: "%Y%m%d_%H%M%S"examples/tracing/agent/mcp_agent.config.yaml (1)
29-34: Typed exporters look good. Add commented OTLP headers hint?Since many collectors require auth or custom headers, a commented example can reduce misconfig.
exporters: - type: console - type: file # To export to a collector, also include: # - type: otlp - # endpoint: "http://localhost:4318/v1/traces" + # endpoint: "http://localhost:4318/v1/traces" + # # headers are merged from secrets; example: + # # headers: + # # Authorization: "Bearer ${OTEL_TOKEN}"src/mcp_agent/mcp/mcp_aggregator.py (5)
349-365: allowed_tools access may break after config changes.PR summary notes removal of
MCPServerSettings.allowed_tools. Directly accessing.allowed_toolscould raise AttributeError at runtime.Defensive read and set normalization:
- else: - allowed_tools = self.context.server_registry.get_server_config( - server_name - ).allowed_tools + else: + cfg = self.context.server_registry.get_server_config(server_name) + allowed_tools = getattr(cfg, "allowed_tools", None) + if isinstance(allowed_tools, list): + allowed_tools = set(allowed_tools)And update membership test stays O(1).
Also applies to: 371-378
41-43: Global logger is mutated per instance; make it instance-scoped.Overwriting the module-level logger in init can cause cross-instance interference. Prefer
self.logger.-logger = get_logger( - __name__ -) # This will be replaced per-instance when agent_name is available +logger = get_logger(__name__) @@ - global logger - logger_name = f"{__name__}.{name}" if name else __name__ - logger = get_logger(logger_name) + logger_name = f"{__name__}.{name}" if name else __name__ + self.logger = get_logger(logger_name)Follow-up: swap
logger.calls in methods toself.logger.(can be done incrementally).Also applies to: 127-131
552-555: Typo in span name (“get_capabilitites”).Minor naming nit; fix for searchability.
- with tracer.start_as_current_span( - f"{self.__class__.__name__}.get_capabilitites" - ) as span: + with tracer.start_as_current_span( + f"{self.__class__.__name__}.get_capabilities" + ) as span:
310-317: Redundant reload in create().
__aenter__()already callsinitialize()which loads servers. The extraload_servers()immediately after is a no-op and adds noise.try: await instance.__aenter__() - - logger.debug("Loading servers...") - await instance.load_servers() - logger.debug("MCPAggregator created and initialized.") return instance
646-648: Docs say “dot-namespaced” but SEP is “_”.Either update docs to “underscore-namespaced” or change SEP if dot is intended.
- :return: Tools from all servers aggregated, and renamed to be dot-namespaced by server name. + :return: Tools from all servers aggregated, and renamed to be underscore-namespaced by server name.examples/tracing/langfuse/README.md (2)
49-56: Confirm secrets path for headers after removing otlp_settings from config.Doc shows headers under
otel.otlp_settings.headersin secrets while config now uses typed exporters. Verify the loader still reads this secrets path and merges into the new OTLP exporter; if not, update the path or include an example of setting headers directly on the exporter (and document precedence).
58-66: Show multi-exporter usage and EU endpoint to reflect the PR’s goal.Consider expanding the snippet to demonstrate multiple exporters (console + OTLP, optionally file) and include an EU endpoint example.
otel: enabled: true exporters: - - type: otlp - endpoint: "https://us.cloud.langfuse.com/api/public/otel/v1/traces" + - type: console + - type: otlp + endpoint: "https://us.cloud.langfuse.com/api/public/otel/v1/traces" + # - type: otlp + # endpoint: "https://eu.cloud.langfuse.com/api/public/otel/v1/traces" + # - type: file + # path: "./traces.jsonl"examples/workflows/workflow_intent_classifier/mcp_agent.config.yaml (1)
24-29: Typed exporters LGTM.Matches the new schema and unlocks multiple exporters. Optionally add a commented example for file and OTLP to guide users.
otel: enabled: false exporters: - type: console + # - type: file + # path: "./traces.jsonl" # To export to a collector, also include: # - type: otlp # endpoint: "http://localhost:4318/v1/traces"examples/workflows/workflow_parallel/mcp_agent.config.yaml (1)
28-33: Consistent migration to typed exporters.Looks good. Consider mirroring a multi-exporter example here as well for parity with other examples.
tests/mcp/test_mcp_aggregator.py (4)
873-876: Type the mock config for clarity.Annotate
allowed_toolsto make the intent explicit.-class MockServerConfig: +class MockServerConfig: """Mock server configuration for testing""" - def __init__(self, allowed_tools=None): - self.allowed_tools = allowed_tools + def __init__(self, allowed_tools: "set[str] | None" = None): + self.allowed_tools = allowed_tools
879-913: Solid dummy registry; minor typing nits.Add return type annotations on helpers to aid IDEs and future refactors.
- def get_server_config(self, server_name): + def get_server_config(self, server_name: str) -> MockServerConfig: return self.configs.get(server_name, MockServerConfig()) - def start_server(self, server_name, client_session_factory=None): + def start_server(self, server_name: str, client_session_factory=None): class DummyCtxMgr:
1033-1071: Assert the warning when allowed_tools is an empty set.Capture and assert the log to prevent regressions.
-async def test_tool_filtering_empty_allowed_tools(): +async def test_tool_filtering_empty_allowed_tools(caplog): ... - await aggregator.load_server("test_server") + with caplog.at_level("WARNING"): + await aggregator.load_server("test_server") + assert any( + "Allowed tool list is explicitly empty" in r.message for r in caplog.records + )
1077-1114: Also assert fallback warning when no server registry is present.-async def test_tool_filtering_no_server_registry(): +async def test_tool_filtering_no_server_registry(caplog): ... - await aggregator.load_server("test_server") + with caplog.at_level("WARNING"): + await aggregator.load_server("test_server") + assert any( + "no tool filter will be applied" in r.message.lower() for r in caplog.records + )src/mcp_agent/tracing/tracer.py (1)
119-147: OTLP exporter merge logic is sound.Merges endpoint/headers from typed config with deprecated otlp_settings. Consider optionally falling back to OTEL_EXPORTER_OTLP_ENDPOINT/HEADERS env vars in a follow-up.
src/mcp_agent/config.py (2)
513-521: Use default_factory for mutable exporters field.Avoids sharing a mutable list across instances.
- exporters: List[OpenTelemetryExporterSettings] = [] + exporters: List[OpenTelemetryExporterSettings] = Field(default_factory=list)
536-586: Backward-compat coercion is solid; add a warning for unknown strings.Unknown exporter literals are silently dropped. A warning helps users spot typos.
for name in exporters: if name == "console": typed_exporters.append({"type": "console"}) elif name == "file": typed_exporters.append( { "type": "file", "path": legacy_path, "path_settings": legacy_path_settings, } ) elif name == "otlp": typed_exporters.append( { "type": "otlp", "endpoint": (legacy_otlp or {}).get("endpoint"), "headers": (legacy_otlp or {}).get("headers"), } ) + else: + warnings.warn( + f"Unknown exporter '{name}' in 'otel.exporters'; ignoring.", + stacklevel=2, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (25)
examples/tracing/agent/README.md(1 hunks)examples/tracing/agent/mcp_agent.config.yaml(1 hunks)examples/tracing/langfuse/README.md(3 hunks)examples/tracing/langfuse/mcp_agent.config.yaml(1 hunks)examples/tracing/langfuse/mcp_agent.secrets.yaml.example(1 hunks)examples/tracing/llm/README.md(1 hunks)examples/tracing/llm/mcp_agent.config.yaml(1 hunks)examples/tracing/mcp/README.md(1 hunks)examples/tracing/mcp/mcp_agent.config.yaml(1 hunks)examples/tracing/temporal/README.md(1 hunks)examples/tracing/temporal/mcp_agent.config.yaml(1 hunks)examples/workflows/workflow_deep_orchestrator/mcp_agent.config.yaml(1 hunks)examples/workflows/workflow_evaluator_optimizer/mcp_agent.config.yaml(1 hunks)examples/workflows/workflow_intent_classifier/mcp_agent.config.yaml(1 hunks)examples/workflows/workflow_orchestrator_worker/mcp_agent.config.yaml(1 hunks)examples/workflows/workflow_parallel/mcp_agent.config.yaml(1 hunks)examples/workflows/workflow_router/mcp_agent.config.yaml(1 hunks)schema/mcp-agent.config.schema.json(5 hunks)src/mcp_agent/cli/cloud/commands/auth/whoami/main.py(0 hunks)src/mcp_agent/cli/cloud/main.py(1 hunks)src/mcp_agent/config.py(3 hunks)src/mcp_agent/mcp/mcp_aggregator.py(1 hunks)src/mcp_agent/tracing/tracer.py(2 hunks)tests/mcp/test_mcp_aggregator.py(7 hunks)tests/test_tracing_isolation.py(4 hunks)
💤 Files with no reviewable changes (1)
- src/mcp_agent/cli/cloud/commands/auth/whoami/main.py
🧰 Additional context used
🧬 Code graph analysis (4)
src/mcp_agent/mcp/mcp_aggregator.py (3)
src/mcp_agent/app.py (3)
context(145-150)server_registry(157-158)logger(189-204)src/mcp_agent/logging/logger.py (2)
warning(156-164)debug(136-144)tests/mcp/test_mcp_aggregator.py (1)
get_server_config(890-891)
src/mcp_agent/tracing/tracer.py (3)
src/mcp_agent/app.py (2)
logger(189-204)session_id(185-186)src/mcp_agent/logging/logger.py (1)
error(166-174)src/mcp_agent/tracing/file_span_exporter.py (1)
FileSpanExporter(16-73)
tests/test_tracing_isolation.py (1)
src/mcp_agent/config.py (3)
Settings(664-771)OpenTelemetrySettings(506-585)FileExporterSettings(484-487)
tests/mcp/test_mcp_aggregator.py (1)
src/mcp_agent/mcp/mcp_aggregator.py (2)
initialize(150-196)load_server(329-458)
🔇 Additional comments (22)
examples/tracing/temporal/README.md (1)
52-58: Confirm endpoint path expectation (/v1/traces vs base URL).Does your exporter builder expect a base endpoint (http://localhost:4318) and append /v1/traces automatically, or a full path? Mismatch can cause 404s or double paths.
If base URL is expected, apply:
- endpoint: "http://localhost:4318/v1/traces" + endpoint: "http://localhost:4318"src/mcp_agent/cli/cloud/main.py (1)
16-22: LGTM on import formatting.Multiline import improves readability and aligns with common style tools. No functional changes.
examples/workflows/workflow_router/mcp_agent.config.yaml (2)
24-28: Typed exporters migration looks good.Console exporter syntax matches the new discriminated union.
24-28: Confirm OTLP exporter endpoint path
Verify if theendpointfield should include the/v1/tracessuffix or only the base URL, and update this example to align with the schema’s handling.examples/tracing/temporal/mcp_agent.config.yaml (1)
48-51: Good migration to typed exporters.Order and syntax look correct; OTLP endpoint provided.
examples/tracing/llm/README.md (1)
13-23: Clearer, copy-pastable OTEL config.Good swap to a full YAML snippet with typed exporters.
examples/workflows/workflow_evaluator_optimizer/mcp_agent.config.yaml (1)
29-33: Typed exporters change looks correct.Matches the new schema; console exporter retained.
examples/workflows/workflow_orchestrator_worker/mcp_agent.config.yaml (1)
29-33: Migration to typed exporters is consistent.Console exporter format aligns with the new config model.
examples/tracing/langfuse/README.md (1)
3-3: Good shift to typed exporter language.Clear and consistent with the repo-wide migration.
tests/mcp/test_mcp_aggregator.py (4)
919-977: Filtering test covers exact include/exclude and namespacing.Good assertions on both per-server map and namespaced map.
983-1018: No-filter path validated well.Coverage looks good.
1120-1220: Multi-server matrix test is comprehensive.Nice coverage of include lists and namespacing across servers.
1223-1277: Edge-case exact-match test is on point.Catches common substring pitfalls.
tests/test_tracing_isolation.py (2)
9-9: Importing FileExporterSettings is correct and aligns with the new typed exporter API.
296-302: LGTM on migrating to typed file exporters in tests.Passing FileExporterSettings with explicit path/path_settings matches the new discriminated-union config and keeps tests isolated per app.
Also applies to: 304-310, 377-383, 451-457
src/mcp_agent/tracing/tracer.py (3)
95-104: Sampler wiring looks good.Clamping sample_rate and using ParentBased(TraceIdRatioBased(...)) is correct and honors parent sampling decisions.
106-113: Robust handling of legacy vs typed exporter entries.Gracefully supports strings and typed configs; unknown types fall through to the error path below.
151-171: File exporter configuration is correct.custom_path and path_settings are forwarded to FileSpanExporter; per-app session_id ensures isolation.
schema/mcp-agent.config.schema.json (3)
305-353: Exporter schema definitions (console/file) look correct and minimal.Using const type fields and allowing optional path/path_settings is aligned with the runtime model.
798-837: OTLPExporterSettings schema is well-formed.type discriminator, endpoint, and headers match tracer usage.
931-949: Discriminated union for otel.exporters is correct.mapping and oneOf cover console, file, and otlp.
src/mcp_agent/config.py (1)
480-488: Typed exporter classes are defined cleanly and match the schema.No issues spotted with type literals and fields.
| tracer_provider.add_span_processor( | ||
| BatchSpanProcessor( | ||
| ConsoleSpanExporter(service_name=settings.service_name) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
ConsoleSpanExporter called with an invalid argument (will raise TypeError).
ConsoleSpanExporter doesn't accept service_name. This will break when exporters include "console".
Apply this fix:
- BatchSpanProcessor(
- ConsoleSpanExporter(service_name=settings.service_name)
- )
+ BatchSpanProcessor(ConsoleSpanExporter())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tracer_provider.add_span_processor( | |
| BatchSpanProcessor( | |
| ConsoleSpanExporter(service_name=settings.service_name) | |
| ) | |
| ) | |
| tracer_provider.add_span_processor( | |
| BatchSpanProcessor(ConsoleSpanExporter()) | |
| ) |
🤖 Prompt for AI Agents
In src/mcp_agent/tracing/tracer.py around lines 114 to 118, the code passes
service_name to ConsoleSpanExporter which does not accept that argument and will
raise a TypeError; remove the service_name argument from ConsoleSpanExporter and
instead ensure the TracerProvider is created/updated with a Resource that sets
"service.name" to settings.service_name (i.e., construct or merge a Resource
containing {"service.name": settings.service_name} and supply it to the
TracerProvider before adding the BatchSpanProcessor using ConsoleSpanExporter()
with no args).
| } | ||
| ], | ||
| "default": null | ||
| "description": "Deprecated single OTLP settings. Prefer exporters list with type \"otlp\"." |
There was a problem hiding this comment.
What's the intention for supporting the deprecated settings vs just enforcing the new ones going forward?
There was a problem hiding this comment.
Deprecating for one release to give people time to migrate, and then will remove in a subsequent change
There was a problem hiding this comment.
Just best practice for a package. Can remove now as well, but it's relatively lightweight to support back-compat right now
|
|
||
| class OTLPExporterSettings(OpenTelemetryExporterBase): | ||
| type: Literal["otlp"] = "otlp" | ||
| endpoint: str | None = None |
There was a problem hiding this comment.
Should this be required str?
| api_key: anthropic_api_key | ||
|
|
||
| otel: | ||
| # Headers are merged with typed OTLP exporter settings |
There was a problem hiding this comment.
How would this look for the new format? Would we need to duplicate the -type and endpoint here to map to the correct one (in case of multiple)?
There was a problem hiding this comment.
Yes, headers would need to be specified for each endpoint, which I think is correct.
class OTLPExporterSettings(OpenTelemetryExporterBase):
type: Literal["otlp"] = "otlp"
endpoint: str | None = None
headers: Dict[str, str] | None = None
| allowed_tools = self.context.server_registry.get_server_config(server_name).allowed_tools | ||
| allowed_tools = self.context.server_registry.get_server_config( | ||
| server_name | ||
| ).allowed_tools |
There was a problem hiding this comment.
Isn't this removed above now?
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/workflows/workflow_evaluator_optimizer/mcp_agent.config.yaml(2 hunks)schema/mcp-agent.config.schema.json(18 hunks)src/mcp_agent/config.py(3 hunks)
2 participants
Clean up
OpenTelemetrySettingsto be able to specify multiple exporters. This allows you to output to console, file, and even multiple otlp collector endpoints.Particularly nice for MCP-agent cloud, where a developer can configure their agent to export to their own OTEL endpoint (while still allowing us to configure out internal OTLP endpoint for surfacing logs to them).
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests